shell: keep variables in one table with an export attribute, and prefix assignments on their command by robobun · Pull Request #45371 · oven-sh/bun

robobun · 2026-10-15T15:54:20Z

Fixes #32202

Problem

T=5; export T; printenv T prints an empty line. export PATH; uname and PATH=/x:$PATH; tool fail with bun: command not found. HOME=/tmp/zz env prints two HOME= lines.
Three maps hold values, and "exported" means "in export_env". export NAME writes "" there (builtin/export.rs:35), and NAME=v writes only shell_env (interpreter.rs:2008). Each spawn appends cmd_local_env, which no command clears (states/Cmd.rs:474).

Fix

One VarTable replaces shell_env and export_env. Each entry is Exported(value), Local(value) or ExportedUnset. An assignment keeps the export state, and export NAME keeps the value.
A NAME=v cmd prefix lives on its Cmd node and ends with it. VarTable::child_env gives the child one entry per name.
Verified: bunshell.test.ts and shell-worker-terminate-leak.test.ts (28 tests fail on main's source), then test/js/bun/shell. One test compares 168 sequences with sh.
Self-reviewed: 19 concerns raised, 19 addressed.

Background

A variable has a value and an export attribute. A child gets each exported variable that has a value, plus its command's prefixes.
Considered: both maps behind a set/export pair, and clearing cmd_local_env per command (shell: replace, don't append, when a prefix assignment overrides an env var #36513, shell: resolve commands on the shell environment's PATH, not the startup snapshot, and scope VAR=value prefixes to one command #42049). Each keeps a second store.

Downsides

Behavior change: an assignment to an inherited name reaches later commands, and a prefix no longer does. On Windows path=dist now replaces Path for children and lookup.
Cost: .text +4,144 B, and +864 B per environment copy at 64 variables. A spawn costs 9 more instructions per unexported variable, and a prefix 4 more allocator calls.
Still wrong: which, cd and a later prefix (A=1 B=$A cmd) ignore a command's prefixes. A=1 $EMPTY assigns nothing.

Notes

Why the prefix store is in this PR. The first pushes left cmd_local_env alone. That left the second bug half fixed. With NODE_ENV inherited, NODE_ENV=test true; NODE_ENV=production; bun -e 'console.log(process.env.NODE_ENV)' printed test: the plain assignment reached the child, and the stale prefix of the earlier command came after it in the environment. bun and sh take the last of two entries, node and python the first. The prefix change is commits 07bcb8a and f51d531. This PR therefore covers #36513 (closed as stale on 2026-10-31) and the prefix part of #42049. The PATH lookup part of #42049 is not here.

History. This replaces #33549 (bare export NAME) and #33550 (assignment to an exported name). A sweep of stale pull requests closed both on 2026-09-13 with no maintainer comment. The review of #33550 found two shadow regressions in a row (FOO=a; export FOO=b; FOO=c; echo $FOO printed a), each repaired by one more "remove it from the other map" at one writer. The tests here reuse the cases of both. #32202 has four earlier attempts, all closed unmerged: #32203, #32205, #33364 and #36513. A later report in the chat thread named the most visible form of the bare export bug: export PATH; uname gives bun: command not found: uname, and export HOME && cd gives cd: HOME not set. The report says 1.3.14 ran both, because its command lookup did not read the shell's PATH. I checked 1.4.3-canary and this branch, not 1.3.14.

What changes for a user.

T=5; export T; cmd: the child gets T=5. It got T= before.
export NAME on an inherited name keeps the value. It blanked it before, for the child and for $NAME. So export PATH; cmd finds cmd, and export HOME && cd works.
export NAME on an unset name adds nothing to the environment. A later NAME=1 is exported.
NAME=value on an inherited or exported name reaches child processes, which, the command lookup on POSIX, the export listing, subshells, pipelines and $( ).
NAME=v cmd on an exported name gives the child one NAME=v entry. It got the old entry and the new one before.
NAME=v cmd1; cmd2: cmd2 does not get NAME=v. It did before.
PATH=x cmd1; cmd2 looks cmd2 up on the shell's PATH. A script that relied on the leaked prefix now gets command not found.
FOO=a; export FOO=b; echo $FOO prints b. PWD=junk; cd /tmp; echo $PWD prints /tmp.
A=1 export A; echo $A prints 1. A prefix on export assigns only a name that is an operand, as in bash.
export with no operand prints a bare NAME for an exported name that has no value.
On Windows, variable names are ASCII-case-insensitive, as in process.env: PATH, Path and path are one variable, so an assignment under any spelling changes what child processes receive. On main that was true for $NAME and not for the child. tmp=x now changes the inherited TMP, pwd=saved; cd sub; echo $pwd prints the new directory, and path=dist; node build.js fails with bun: command not found: node. PATH=dir; tool finds tool there: the command lookup matches the key by the same rule since 3ad5807 (name_eql), where it matched PATH byte for byte before. The alternative is a different design: upper-case the inherited names at startup and keep shell names case-sensitive, as Git Bash does.

Code removed. ShellExecEnv::shell_env, export_env and cmd_local_env, ShellExecEnv::assign_var, EnvMap::{iterator, iter, clone, init_with_capacity, ensure_total_capacity, memory_cost}, and the second fill_env call per spawn. EnvMap is now only the prefix map of one command.

Sequences against dash 0.5.12-12. A one-off measurement on 2026-10-16. Its generator is not in the tree. The check that stays in the tree is the test sequences of assignments, exports and prefixes give the result of sh: 84 sequences for an unset name and 84 for an inherited one, against the system sh. The matrix: every sequence of 1 to N operations on one name, observed through printenv in a child and through $NAME in the shell. "Before" is a release build of main a407eae. "After" is this branch at f51d531, release build.

Matrix	Operations	Before
assignment and export, name unset or inherited	`A=1`, `A=2`, `A=`, `export A`, `export A=3`, `export A=`, up to 4	2,230 of 3,108
`cd`, with `PWD` inherited	`PWD=junk`, `OLDPWD=junk`, `cd /tmp`, `cd /usr`, `export OLDPWD`, up to 4	696 of 780
subshell, pipeline member, `$( )`	7 operations, up to 3	624 of 798
prefix on `export`, shell view only	`A=1 export A`, `A=2 export A=3 A`, `A=4 true`, `export A`, `A=5`, up to 4	1,026 of 1,560
prefix on a command, a builtin and `export`	`A=1 true`, `A=2 printenv A`, `A=3 export A`, `export A`, `A=5`, `export A=6`, up to 4	3,074 of 3,108

The cd row holds only with PWD in the starting environment. Without it, 120 of the 780 differ, because $PWD is empty until the first cd (see "Not fixed here").
The matrices use one name. A prefix whose value reads another prefix (A=1 B=$A cmd) is outside them.
dash and bash disagree on 160 of the 780 cd sequences, for two reasons. bash marks OLDPWD exported at the start (26). The cd of bash copies the variable $PWD into OLDPWD, and dash stores the previous directory (134). On the other matrices the two agree.
Where bash and dash disagree, this PR keeps what main does: dash for cd, and bash for a prefix on export (only an operand is assigned). A test records each of the two.
A further 22 hand-written prefix scripts (pipeline member, subshell, $( ), redirect, &&, if, command not found, two prefixes of one name) give the dash result, except one that uses "$?", which Bun Shell does not expand. 20 of the 22 differ on main.

Tests.

28 tests fail with git checkout --no-overlay origin/main -- src/ packages/ and pass with the branch's source (debug build, ASAN, after the merge of main 2596ac5). They are the exported variables block, export var, external command resolution uses the PATH from the shell environment and the LeakSanitizer case.
bunshell.test.ts is on the list of files that run with LeakSanitizer off (test/no-validate-leaksan.txt). The new case in shell-worker-terminate-leak.test.ts runs with it on. It takes each release path of the variable table and of a prefix map, and ends with a live command. With three deref calls deleted (in VarTable::set, EnvMap::insert and Export::assign_prefixes) it reports three leaked strings.
Windows: I cannot run Windows here. CI build 138615 ran the Windows tests at 3ad5807 on x64 and arm64, and all 16 test jobs passed: a name in another case is the same variable, cmd /d /c set NAME shows one entry for a name that a prefix replaces, and which and the command lookup find a tool after PATH=dir. The Windows target also type-checks (bun run rust:check-all x86_64-pc-windows-msvc).
bunshell.test.ts and shell-worker-terminate-leak.test.ts on the branch: 501 pass, 0 fail.
All 44 files of test/js/bun/shell in one bun bd test run: every failure on the branch is a timeout of a slow test on the debug build (fd leak, shell-hang, shell load, ls of node_modules, bun exec > it works2, and once commands/cat.test.ts, which passes alone 3 of 3).

Not fixed here.

Four readers do not see the prefixes of their own command. The prefixes live on Cmd.prefix_env. The child environment, the command lookup and the operands of export read it. which (PATH=x which tool), cd (HOME=/x cd), $NAME in a later prefix (A=1 B=$A cmd, A=1 B=$(printenv A) cmd) and a command with no command word (A=1 $EMPTY, which assigns nothing) do not. All four give the same result on main. One follow-up should fix them together, after shell: keep an empty word that has a double-quoted expansion #44904 and shell: record the last command substitution status per command and expand words first #45074, which change when the words of a command expand. Its shape is open: one lookup that checks the prefixes first, or the prefixes applied to the table for the life of the command. For the command with no command word, the fix belongs in the AssignCtx::Cmd arm of Assigns, not in AssignCtx::Shell. A todo test records that case.
The shell does not set PWD from its working directory at startup. Without .cwd() (or bun run <script>, which passes the package directory), $PWD and the PWD of a child are what the starting environment holds: missing when it has none, stale when it names another directory, until the first cd.
Two bash behaviours: OLDPWD is exported from the start, and cd copies $PWD into OLDPWD.
The command lookup and which read only an exported PATH. sh also uses an unexported one (shell: resolve commands on the shell environment's PATH, not the startup snapshot, and scope VAR=value prefixes to one command #42049).
export -p, export -n and invalid names (shell: export rejects invalid bare names, an empty name and options #40710, which rewrites Export::start and names this PR for the bare NAME arm. The second of the two to land needs a rebase).

Open pull requests in the same files.

shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr #40262 (on shell: remove unsafe from the interpreter, state nodes, IOWriter/IOReader and Builtin #40228) changes all nine Rust files of this PR. It makes EnvStr a Clone-only enum over Rc<[u8]>, with no ref_() or deref(). VarTable as written here does not compile on it: Var loses Copy, set and export take the value by move, get returns a borrow, and the refcount loops in clone and Drop go away. The self-review ported it: about 118 of the 267 added source lines change, in either merge order (compile-checked only, not run). That stack is 728 commits behind main 2596ac5. In change_cwd_impl, keep the dupe_ref_counted of shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr #40262: this PR still has main's init_slice there.
shell: replace EnvStr packed u128 with a niche enum over NonNull #37608 changes only the representation of EnvStr. Var holds an EnvStr by value and follows it.
Conflicted files, this branch against main (git merge-tree, 2026-11-08): shell: export rejects invalid bare names, an empty name and options #40710, Bun shell: use the passwd home for ~ when HOME is unset #43202 and shell: keep an empty word that has a double-quoted expansion #44904: 1 against 0. shell: record the last command substitution status per command and expand words first #45074: 1 against 1. shell: resolve commands on the shell environment's PATH, not the startup snapshot, and scope VAR=value prefixes to one command #42049: 5 against 2. shell: replace EnvStr packed u128 with a niche enum over NonNull #37608: 2 against 2. shell: remove unsafe from the interpreter, state nodes, IOWriter/IOReader and Builtin #40228: 55 against 54. shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr #40262: 60 against 59.
A rebase maps as follows: export_env.get becomes vars.get_exported, shell_env then export_env becomes vars.get, and the two fill_env calls become vars.child_env(prefixes).

Measurements. Taken on 2026-10-16, between main a407eae and this branch at f51d531, before the merge of main 2596ac5. Release builds with --lto=off, env -i with 64 pinned variables, bun run script.bun.sh. valgrind, perf, strace and bloaty are not installed in the build container, and perf_event_open is not permitted there. The counts come from gdb breakpoints on function entry addresses. A "per statement" number is the difference of a 200-statement and a 100-statement script, divided by 100.

release binary: .text +4,144 B (76,512,176 to 76,516,320), file size unchanged at 101,439,328 B (llvm-size, stripped).
environment fill per spawn, instructions from the start of the fill in Cmd::transition_to_exec to the call of bun_which::which: with 0 unexported variables main 18,812, PR 18,216. With 100: 18,782, 19,231. With 1,000: 18,790, 27,323. Slope: 9.1 instructions per unexported variable (main 0). The array of entry pointers is reserved once now. It grew by doubling before.
the same span for a command with one prefix: a new name, main 25,663, PR 26,526. A name that replaces an exported one, main 25,665, PR 27,615. Each exported name is compared with each prefix name.
a command with a prefix (A=1 true): main 15.03 allocator calls and 1,358 B per statement, PR 19.03 calls and 1,582 B. The prefix map of a command is a box and three arrays, made for that command. main reuses one map.
env copy at 64 variables: main 21.03 allocator calls / 4,685 B per (true); statement, PR 21.03 calls / 5,549 B. Per $ start (Bun.$ from a JS file, two runs each): main 373 calls / 27.1 to 27.4 KB, PR 373 calls / 28.1 KB.
sizes (debug builds, gdb): Var is 32 B, EnvStr is 16 B. ShellExecEnv: main 368 B, PR 192 B. Cmd and the state node Node: main 176 B, PR 184 B (the prefix map is an Option<Box<EnvMap>>).
first fresh assignment per environment: main 3 calls / 144 B (mi_malloc), PR 3 calls / 6,656 B (mi_realloc, the table grows from 64 to 128 entries). Later fresh assignments: 6.08 allocator calls per statement on both. Requested bytes per statement over the first 100: main 636 B, PR 823 B.
name hashes per $NAME (inherited): main 2, PR 1. Unset name: 2, 1. ~: 2, 1. Plain shell variable: 1, 1. Fresh NAME=v, reassignment, export NAME=v, which: 1 on both. cd: 2 on both. (Debug builds at be4b33c, breakpoint on EnvMapContext::hash. The later commits do not touch these paths.)
syscalls for A=1; export A; /usr/bin/true; (true); echo $(echo x), three runs each, at be4b33c: main 396, 396, 399. PR 394, 394, 399. Only thread and event-loop calls differ, between builds and between runs of one build. Host functions added: 0.
An earlier push kept the export state in two spare bits of the EnvStr tag (16 B entry, no extra bytes per copy). The first self-review rejected it: it is bit-packing where a plain enum is correct, and shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr #40262 and shell: replace EnvStr packed u128 with a niche enum over NonNull #37608 both replace the packed EnvStr.

Self-review. Two reviews. The first asked for a restructure, with two findings: no export state in EnvStr tag bits (done in be4b33c), and cmd_local_env was still a second store (done in 07bcb8a). The second read the final diff and said to keep this shape. It raised 19 concerns, none about the design: 9 about tests, Windows and claims in this description, and 10 smaller ones. All 19 are addressed: the tests and the docs sentence in 08e204f, the rest in this description and in a follow-up that is handed off. A bot review of the merged head then raised 7 findings. Two are fixed in 3ad5807 (the PATH key of the Windows lookup, and the reference shell run of the sequence test). The one on Windows name case is open for a maintainer. Four have replies: two are optional and two exist on main.

no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/shell-worker-terminate-leak.test.ts, test/js/bun/shell/bunshell.test.ts

robobun · 2026-10-15T15:54:35Z

^{Updated 6:27 AM PT - Nov 8th, 2026}

❌ @robobun, your commit 3ad5807 has some failures in Build #138615 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 45371

That installs a local version of the PR into your bun-45371 executable, so you can run:

bun-45371 --bun

robobun · 2026-10-15T15:54:39Z

Status. Reproduced on the released 1.4.3-canary (367d939) and on builds of main, with dash 0.5.12 and bash 5.2.37 as the reference:

T=5; export T; printenv T          prints an empty line (sh: 5)
export PATH; uname                 bun: command not found: uname (sh runs it)
OUTER=changed; printenv OUTER      prints the inherited value (sh: changed)
PATH=/tmp/xbin:$PATH; mytool       bun: command not found: mytool (sh runs it)
HOME=/tmp/zz env                   prints two HOME= lines (sh: one)
OUTER=pre true; OUTER=changed; bun -e 'console.log(process.env.OUTER)'
                                   prints pre (sh: changed)

This pull request is ready for review. It has main 2596ac5 merged in.

CI build 138615 on 3ad5807: no test of this change failed, and 185 of 188 jobs passed, with every Linux, Windows and macOS arm64 lane. The build is red because of the macOS x64 lane. Two of its four test jobs expired after 7 hours without an agent. One stopped when the release directory with the binary under test disappeared on the agent (ENOENT ... lstat .../builds/darwin-x64-14-8-9-1/bun/bun/release).

Open for a maintainer: the three questions in #45371 (comment). The review thread on Windows name case is the second of them.

…ent of path spelling

…n EnvStr tag bits

The prefixes of `NAME=value cmd` were in `cmd_local_env`, a map on the shell environment that no command cleared. Every later command of the same shell got them, and the spawn code appended them after the exported variables, so a name in both reached the child twice. The prefixes now live on the `Cmd` node (`prefix_env`) and go away with it. `VarTable::child_env` yields each exported name once, and a prefix replaces the exported value. `fill_env` takes that one iterator and reserves the whole array, with the null that ends it, in one call.

…prefixes by operand `VarTable::child_env` hashed every exported name to ask the prefix map whether a prefix replaces it. A command has few prefixes, so it now compares each exported name with the prefix names. `export` looked for its own prefixes through the labels of the AST and compared bytes. It now asks the prefix map for each operand name, which compares names the way the variable table does.

…s and the release paths - `cd`: the test reads PWD and OLDPWD in a child, with neither inherited. - Two tests record a choice between shells: a prefix on `export` assigns only an operand (bash), and OLDPWD is exported from the first `cd` (dash). - Windows: a name in another case is the same variable, and `cmd /c set` shows one entry for a name that a prefix replaces. - The PATH test is two tests. `which` runs on every platform. The lookup is a todo on Windows, where it matches the key `PATH` byte for byte. - A `PATH=` prefix does not decide where a later command is found. - 168 sequences of assignments, exports and prefixes per name are compared with the system `sh`. - A LeakSanitizer case in shell-worker-terminate-leak.test.ts takes each release path of the variable table and of a command's prefix map. - docs: variable names are ASCII-case-insensitive on Windows.

…l-export-attribute # Conflicts: # src/runtime/shell/ParsedShellScript.rs # src/runtime/shell/builtin/export.rs # src/runtime/shell/interpreter.rs # src/runtime/shell/states/Cmd.rs

…nd for the command lookup

coderabbitai · 2026-11-08T05:19:03Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

Source mode: Auto
Repository: oven-sh/bun
Branch: robobun/06b9ce56/shell-export-attribute
File: .coderabbit.yaml
Commit: 3ad5807
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 65d01fe1-27a8-4929-8807-f6cfbbbc1f88

Commits

Reviewing files that changed from the base of the PR and between 2596ac5 and 3ad5807.

Files selected for processing: 12

docs/runtime/shell.mdx
src/runtime/shell/EnvMap.rs
src/runtime/shell/ParsedShellScript.rs
src/runtime/shell/builtin/export.rs
src/runtime/shell/builtin/which.rs
src/runtime/shell/interpreter.rs
src/runtime/shell/states/Assigns.rs
src/runtime/shell/states/Cmd.rs
src/runtime/shell/states/Expansion.rs
src/runtime/shell/subproc.rs
test/js/bun/shell/bunshell.test.ts
test/js/bun/shell/shell-worker-terminate-leak.test.ts

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

robobun · 2026-11-08T05:19:06Z

@Jarred-Sumner This is the one pull request for the two lost-value bugs from the thread (export NAME loses the value, and NAME=value on an exported variable does not reach a child). It is ready for review. Three decisions are yours.

1. The prefix change. Commits 07bcb8a and f51d531 move the NAME=v cmd prefixes onto their command. Without them the second bug stays half fixed: with NODE_ENV inherited, NODE_ENV=test true; NODE_ENV=production; bun -e 'console.log(process.env.NODE_ENV)' prints test. The change also fixes #32202, and it changes the export var test, which expected a prefix to reach the next command. If you want only the two bugs you named, I can take the prefix change out.

2. Windows names. The one table compares names without case on Windows, as process.env does. So path=dist now changes the inherited Path that a child gets. On main only $NAME ignored the case. The alternative is to upper-case the inherited names at startup and keep shell names case-sensitive, as Git Bash does.

3. Order against #40262. VarTable uses the Copy EnvStr of main. On your stack it needs a port of roughly 120 lines, in either merge order. The mapping is in a note on #40262.

claude

Beyond the inline findings, I also traced the EnvStr refcounts through VarTable::set/export/clone/Drop and Export::assign_prefixes (the prefixes.get ref is consumed by the single deref after vars.set takes its own), and the new Cmd.prefix_env box is released by EnvMap's Drop when the node is dropped — I found no imbalance there. I did not build or run the test suite; the Windows case-folding and sh differential checks rest on reading only.

Extended reasoning...

The change replaces three EnvMaps in the Bun Shell interpreter with a single VarTable carrying an export attribute, moves per-command NAME=v prefixes onto the Cmd node, and updates export/which/cd, child env construction, docs and tests. No auth, crypto or network surface is touched; the sensitive part is manual EnvStr reference counting and the Windows case-insensitive env key handling. Seven confirmed findings (Windows case-fold collisions, Path spelling on Windows, lost prefix on empty argv, PATH not found when not inherited, test shape) already go inline, so a human look is needed; this note only records the refcount and ownership paths I checked and ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

🟣 src/runtime/shell/subproc.rs — Windows users who assign PATH=...; then run a tool get bun: command not found while the child they do manage to spawn now receives the new value; POSIX finds the tool. child_env filters the inherited Path case-insensitively (EnvMap.rs:225) and emits the stored key spelling, but fill_env updates self.path only when key == b"PATH" byte-exact at subproc.rs:1412, so argv[0] lookup keeps the event-loop PATH. Fix: compare the key with the same case rule as EnvMapContext.eql on Windows (or resolve lookup PATH via vars.get_exported(PATH)), so lookup, which and the child env agree; drop the .todoIf(isWindows) at bunshell.test.ts:1536. [also at: src/runtime/shell/subproc.rs:1414 - Windows users who assign PATH in a script, then run a tool that lives only there, get bun: command not found while the child environment they hand to other programs already says Path=<newdir>.]

Why this was flagged

Trigger: on Windows, a script does PATH=C:\tools;$PATH; mytool or export PATH=...; mytool where mytool is only on the new PATH, through the normal JS $ path where .env() is process.env (key spelled Path). VarTable::set at EnvMap.rs:158 keeps the existing key Path, so child_env at EnvMap.rs:225 yields (Path, newvalue); fill_env at subproc.rs:1412 compares the key byte-for-byte with PATH, so spawn_args.path stays the process PATH from SpawnArgs::default and bun_which::which at Cmd.rs:483 misses the tool. Meanwhile which mytool reports it found, so the user sees which and execution disagree. The base branch never delivered the assignment to the child at all; the PR now delivers it to the child but not to lookup, which is a new inconsistency the .todoIf(isWindows) at bunshell.test.ts:1536 only records. Remedy: match the key case-insensitively on Windows at subproc.rs:1412, or take the lookup PATH from vars.get_exported(PATH).

Verification: Acknowledged in diff: test/js/bun/shell/bunshell.test.ts:1535-1536 with test.todoIf(isWindows). On Windows VarTable::set keeps the inherited key Path (EnvMap.rs:157), child_env yields (Path, newvalue), and fill_env only updates self.path when key == b"PATH" (src/runtime/shell/subproc.rs:1412-1414), so bun_which::which at Cmd.rs:483 fails. On the base the same byte-exact compare already answers "command not found".

claude · 2026-11-08T05:33:10Z

-            value.ref_();
+    /// `NAME=value`: the name keeps its export attribute. Refs `value`, like `EnvMap::insert`.
+    pub(crate) fn set(&mut self, name: EnvStr, value: EnvStr) {
+        let slot = self.map.get_or_put(name).expect("OOM");


🔴 Windows users whose script assigns a lowercase shell variable such as path=dist, os=$(uname) or tmp=... now hand that value to every later child process under the inherited Path/OS/TMP name, breaking those children; on the base branch the child kept the inherited value. VarTable::set at src/runtime/shell/EnvMap.rs:158 looks the name up with the ASCII-case-insensitive EnvMapContext, finds the inherited Exported entry and overwrites its value in place, so child_env emits it. Fix: a plain assignment whose spelling differs from an inherited name must not change what children receive for that name, with the same rule at export (EnvMap.rs:182) and the cd writes of PWD/OLDPWD (interpreter.rs:1981).

Why this was flagged

A Bun Shell script on Windows runs path=./dist; node build.js. The default env is process.env, so the table holds Path, OS, TMP as Var::Exported (src/runtime/shell/ParsedShellScript.rs:112). Assigns::child_done calls shell.vars.set(name, value_ref) (src/runtime/shell/states/Assigns.rs:144); set calls self.map.get_or_put(name) at src/runtime/shell/EnvMap.rs:158, and EnvMapContext::eql (EnvMap.rs:35) compares case-insensitively on Windows, so found_existing is true and the Var::Exported(old) arm at EnvMap.rs:170 replaces the inherited value. child_env (EnvMap.rs:224) then yields Path=./dist to fill_env and node is spawned with a one-entry Path, so anything it spawns (npm, cmd, git) fails with not-found. On the base branch the assignment went to the separate shell_env map (assign_var, old interpreter.rs:2008) and the child was built from export_env only, so it kept the inherited Path/OS. The test "a name in another case is the same variable on Windows only" (test/js/bun/shell/bunshell.test.ts) pins the new behaviour.

Verification: VarTable::set (EnvMap.rs:157-176) does self.map.get_or_put(name) at line 158, so path finds the existing Path entry, and lines 170-173 replace it in place with Var::Exported(value). child_env (EnvMap.rs:224-229) then emits Path=./dist to every later child. On the base, AssignCtx::Shell wrote only shell_env.insert(label, value), so shell_env entries never reached the child.

Kept for now, and it is a maintainer decision (question 2 in #45371 (comment)).

The shell's variable names are ASCII-case-insensitive on Windows on main too: EnvMapContext is the key rule of all three maps. So on main path=dist already changes what $PATH and $Path expand to in the shell. Only the child disagreed with the shell, and that disagreement is the bug this PR fixes.

A rule by spelling cannot keep the fix. The inherited key is Path, so PATH=C:\tools;$PATH is also "a spelling that differs from an inherited name". To keep path=dist local and let PATH=... reach the child, the shell needs another model: names upper-cased at startup and case-sensitive after that, as Cygwin does for a fixed list. That changes the names a child sees.

You are right about the cost, and after 3ad5807 it is larger: on Windows path=dist; node build.js now fails with bun: command not found: node, because the lookup reads the same variable. The description lists this under Downsides, the docs state the rule, and the test a name in another case is the same variable on Windows only records it. I leave this thread open for the maintainer.

One precedent for the other model, for whoever decides. deno_task_shell upper-cases environment names on Windows and keeps shell variables case-sensitive (src/shell/types.rs: apply_env_var upper-cases the name, and EnvChange::SetShellVar updates an environment variable only when the name as written is a key of env_vars). There PATH=x updates the environment and path=dist stays a shell variable. The cost of that model is that a child gets the upper-cased names (PATH, not Path), and that Path=x is a shell variable too.

This PR keeps the rule that main already has for the shell on Windows (one name for every spelling), so the thread stays open as a design question, not as a defect I missed.

…the variable table `fill_env` took the lookup path from the entry whose key is `PATH` byte for byte. On Windows the inherited key is `Path`, and the variable table keeps that spelling, so after `PATH=dir` the child got the new value and the lookup still used the PATH of the process. `name_eql` is now the one rule for variable names (ASCII-case-insensitive on Windows), used by the table, by `child_env` and by the lookup. The lookup test runs on Windows. The sequence test runs the reference `sh` with `Bun.spawn`, beside the Bun Shell run, and checks the exit code and stderr of both.

robobun · 2026-11-08T06:42:35Z

The finding outside the diff is fixed in 3ad5807 (Windows: PATH=dir; tool reached the child and not the command lookup). fill_env matched the key PATH byte for byte, and the variable table keeps the inherited spelling Path. name_eql in src/runtime/shell/EnvMap.rs is now the one rule for variable names, and the table, child_env and the lookup use it. The test a plain assignment to PATH is used by command lookup has no todo on Windows now.

I cannot run Windows here. The Windows target type-checks (bun run rust:check-all x86_64-pc-windows-msvc), and CI runs the test.

Each inline finding has a reply. Five threads are resolved. The thread on Windows name case stays open for the maintainer.

claude

I re-reviewed the latest push (3ad5807) and found no new bugs; because this rewrites how Bun Shell stores and exports variables and codifies a Windows name-folding design choice, a human look is still worthwhile.

What was reviewed on this push:

name_eql now shared by EnvMapContext::eql, child_env and fill_env's PATH detection; the refcount arms of VarTable::set/export and Export::assign_prefixes balance on every path, and Cmd.prefix_env is released when free_node overwrites the node.
The sequence test now runs sh via Bun.spawn with concurrent drains and asserts its exit code and stderr, and the PATH command-lookup test is enabled on Windows.
Not done: no build or test run was performed; the sh differential and the LeakSanitizer case were checked by reading only.

Extended reasoning...

The diff replaces the shell's three environment maps with a single VarTable carrying an export attribute, moves NAME=value prefixes onto their Cmd node, and routes export, which, cd, subshell cloning and child-env construction through it, with tests and docs. It touches no auth, crypto or injection surface, but it changes user-visible shell semantics (what children inherit, PATH lookup, and on Windows which inherited variable a lowercase assignment overwrites, now asserted by a new Windows test). The latest commit addressed the spawnSync nit from the earlier review and unified PATH key matching; the hunt ran dry with no new findings. Deferring rather than approving because the change is large, alters behavior users depend on, and the Windows case-folding choice is a design decision the author themselves lists with an alternative.

Still open from earlier reviews (1):

🔴 src/runtime/shell/EnvMap.rs:160 — Windows users whose script assigns a lowercase shell variable such as path=dist, os=$(uname) or tmp=... now hand that v…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

shell: keep variables in one table with an export attribute

b110471

robobun assigned Jarred-Sumner Oct 15, 2026

github-actions Bot added the claude label Oct 15, 2026

[autofix.ci] apply automated fixes

5277ee9